Fix leaks from unprocessed user filter buckets - #23267

Looks good to me.

The same error message ("Unprocessed filter buckets remaining on input brigade") is present around line 190, if userfilter_assign_stream fails. Is the same cleanup necessary there? I don't think so, since it returns PSFS_ERR_FATAL and then the caller cleans up. Also, the error message seems incorrect, or at least misleading. In any case, that falls outside the scope of this PR.

@iliaal, could you take a look at this? You recently modified the same code in #22850.

Should this target a bug-fix branch instead of master?

The drain is in the right function, but doing it unconditionally destroys data on the read path.

_php_stream_fill_read_buffer() declares brig_in/brig_out outside the while (!stream->eof ...) loop and never reinitialises them, so buckets a filter leaves on $in are re-presented on the next call with the new chunk appended. A filter that returns PSFS_FEED_ME and puts its buckets back is using that. With the patch those buckets are freed before the next call:

class defer extends php_user_filter {
    private int $calls = 0;
    public function filter($in, $out, &$consumed, bool $closing): int {
        $this->calls++;
        $buckets = [];
        while ($b = stream_bucket_make_writeable($in)) { $buckets[] = $b; }
        if ($this->calls < 3) {
            foreach ($buckets as $b) { stream_bucket_prepend($in, $b); }
            return PSFS_FEED_ME;
        }
        foreach ($buckets as $b) { stream_bucket_append($out, $b); }
        return PSFS_PASS_ON;
    }
}
stream_filter_register("defer", "defer");
$fp = fopen($threeChunkFile, 'r');
stream_filter_append($fp, "defer", STREAM_FILTER_READ);
var_dump(strlen(stream_get_contents($fp)));

24576 on master, 8192 with this patch. The buckets are userland-owned after stream_bucket_make_writeable(), so that is real content going away, not the aliasing artifact you get when the buckets are left untouched (master hands those back as three copies of the last chunk, since they all alias the one reused chunk_buf from streams.c:465).

I do not think the deferral pattern is worth preserving as such, but silently truncating a stream is worse than the leak. Gating the drain on ret != PSFS_FEED_ME, or draining only when the filter is finished with the brigade, keeps the leak fixed without the truncation. Worth checking what the PSFS_FEED_ME case in _php_stream_fill_read_buffer() should own here, given php_stream_filter.buffer at php_stream_filter_api.h:130 has no readers or writers anywhere in the tree, so the "filter MUST attach any un-consumed buckets to its own brigade" comment above the swap describes a mechanism that does not exist.

Two smaller things. The buckets_out drain that c087398cc2e removed alongside this one is not restored, so a filter returning PSFS_FEED_ME with output buckets attached still leaks at php_stream_filter_flush(). And under a normal build all three tests pass without the C change; only --enable-debug makes them red-before. Given the sections are --XFAIL-- rather than --XLEAK-- these two currently report WARN, not XFAIL, so it is worth saying in the description that the coverage is debug-build only.

On the second warning site: the caller does not always clean up. php_stream_filter_flush() returns bare at main/streams/filter.c:477 for PSFS_FEED_ME and :480 for PSFS_ERR_FATAL and drains neither brigade, and its PSFS_PASS_ON swap clears outp->head/outp->tail without freeing what was there. php_stream_filter_append_ex() and both streams.c sites do drain on PSFS_ERR_FATAL, so it is three out of four.

On the branch: the leak is on 8.4 and 8.5 too, so the fix belongs on the lowest affected branch rather than master. The behavior question above is the reason to settle it on master first.

Also worth linking up: #20058 does the same thing, has been open since October, and additionally drains buckets_out and removes the now-redundant caller-side drains. The two should not land separately.